Implement CE Subscriptions filters #5715

devguyio · 2021-09-07T06:37:10Z

Part of #5204
Rewrite of #5205 by @slinkydeveloper .

Proposed Changes

🎁 Add CE Subscriptions API filters field to Trigger objects.

Pre-review Checklist

At least 80% unit test coverage
E2E tests for any new behavior
Docs PR for any user-facing impact
Spec PR for any new API feature
Conformance test for any change to the spec

Release Note

Triggers now include a CloudEvents Subscriptions API compatible filters field as an experimental feature.

Docs

Docs will be added with the implementation in mt-broker.

devguyio · 2021-09-08T20:50:55Z

This is ready for review, but I'm holding to figure out how should this be merged wrt the experimental feature in #5204 . I'm also still not clear on docs, spec and rest of the check boxes in the description.
/hold

codecov · 2021-09-08T20:55:07Z

Codecov Report

Merging #5715 (5363c2b) into main (31e4e2e) will increase coverage by 0.05%.
The diff coverage is 82.71%.

@@            Coverage Diff             @@
##             main    #5715      +/-   ##
==========================================
+ Coverage   81.99%   82.05%   +0.05%     
==========================================
  Files         220      220              
  Lines        7458     7527      +69     
==========================================
+ Hits         6115     6176      +61     
- Misses        916      917       +1     
- Partials      427      434       +7

Impacted Files	Coverage Δ
pkg/apis/eventing/v1/trigger_types.go	`100.00% <ø> (ø)`
pkg/apis/eventing/v1/trigger_validation.go	`85.61% <82.71%> (-6.17%)`	⬇️
pkg/scheduler/statefulset/scheduler.go	`78.59% <0.00%> (+0.63%)`	⬆️
pkg/reconciler/subscription/subscription.go	`80.23% <0.00%> (+1.01%)`	⬆️
pkg/channel/message_dispatcher.go	`81.25% <0.00%> (+1.56%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 31e4e2e...5363c2b. Read the comment docs.

antoineco · 2021-09-08T21:12:22Z

If this is only adding API fields (and their validation) but the data plane implementation doesn't exist yet, is it really wise to add release notes already?

I'm also curious whether there is already a formal plan for implementing the "dialects" (part 2 of the feature).

odacremolbap

Nice to see this taking shape.
Did some early comments on it.

config/core/resources/trigger.yaml

pkg/apis/eventing/v1/trigger_types.go

pkg/apis/eventing/v1/trigger_validation.go

docs/eventing-api.md

devguyio · 2021-09-08T21:47:29Z

If this is only adding API fields (and their validation) but the data plane implementation doesn't exist yet, is it really wise to add release notes already?

It was one of the questions I wanted to discuss, cause I also think it probably should only be added once the full experimental feature gets merged.

I'm also curious whether there is already a formal plan for implementing the "dialects" (part 2 of the feature).

Yes, in this section in the feature track which is also linked in the overarching issue #5204

antoineco

Overall this looks solid, I like what this API will enable 👍
I haven't looked at the tests yet, so just focusing on generic stuff and typos spotted in the doc so far.

config/core/resources/trigger.yaml

docs/eventing-api.md

pkg/apis/eventing/v1/trigger_types.go

pkg/apis/eventing/v1/trigger_validation.go

pierDipi

If this is intended to be an experimental feature I think we should follow for consistency a similar pattern of #5149:

The new field is not added to the OpenAPI schema
The feature is disabled by default and uses a feature flag to enable it

devguyio · 2021-09-09T12:56:07Z

If this is intended to be an experimental feature I think we should follow for consistency a similar pattern of #5149:

The new field is not added to the OpenAPI schema

@pierDipi I wasn't aware of the OpenAPI schema requirement, you are right! It was documented in our experimental feature doc 😅 .

The feature is disabled by default and uses a feature flag to enable it

Yeah definitely, it's why I've a hold on this cause I'm still discovering the mechanics of how everything will land together but needed a feedback on the API shape. Cool!

Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com>

devguyio · 2021-11-10T22:14:45Z

Updates:

Applied review comments
Added a feature flag

/unhold

Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com>

config/core/configmaps/features.yaml

pkg/apis/eventing/struct_validation.go

pkg/apis/eventing/v1/trigger_validation.go

Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com>

devguyio · 2021-11-11T14:01:41Z

@pierDipi @antoineco changed. Thanks for the review.

knative-metrics-robot · 2021-11-11T14:06:25Z

The following is the coverage report on the affected files.
Say /test pull-knative-eventing-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/eventing/v1/trigger_validation.go	98.3%	94.1%	-4.2

pierDipi

Let's merge this awesome feature!

/lgtm
/approve

knative-prow-robot · 2021-11-11T14:26:03Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: antoineco, devguyio, pierDipi

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [antoineco,devguyio,pierDipi]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

knative-prow-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 7, 2021

google-cla bot added the cla: yes Indicates the PR's author has signed the CLA. label Sep 7, 2021

knative-prow-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. area/test-and-release Test infrastructure, tests or release approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Sep 7, 2021

devguyio mentioned this pull request Sep 7, 2021

[DUMMY] Why are roundtrip tests failing when adding this field? #5716

Closed

devguyio force-pushed the ce_subscriptions_api_filter branch from cf97e31 to 09e394b Compare September 8, 2021 20:48

devguyio changed the title ~~[WIP] Implement CE Subscriptions filters~~ Implement CE Subscriptions filters Sep 8, 2021

knative-prow-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 8, 2021

knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 8, 2021

odacremolbap reviewed Sep 8, 2021

View reviewed changes

antoineco reviewed Sep 8, 2021

View reviewed changes

pierDipi reviewed Sep 9, 2021

View reviewed changes

devguyio changed the title ~~Implement CE Subscriptions filters~~ [WIP] Implement CE Subscriptions filters Sep 9, 2021

knative-prow-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 9, 2021

devguyio added 4 commits November 10, 2021 16:30

Implement CE Subscriptions filters

145a441

Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com>

add oneof validation to filters

d4f67f2

Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com>

Removing field schema

97f9ef1

Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com>

Adding feature flag

61fc963

Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com>

devguyio changed the title ~~[WIP] Implement CE Subscriptions filters~~ Implement CE Subscriptions filters Nov 10, 2021

knative-prow-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 10, 2021

knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 10, 2021

devguyio force-pushed the ce_subscriptions_api_filter branch from 09e394b to 61fc963 Compare November 10, 2021 22:16

Organize imports

f3e8ff9

Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com>

update codegen

f2b8c8e

Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com>

matzew reviewed Nov 11, 2021

View reviewed changes

config/core/configmaps/features.yaml Show resolved Hide resolved

pierDipi self-requested a review November 11, 2021 07:28

pierDipi reviewed Nov 11, 2021

View reviewed changes

devguyio added 2 commits November 11, 2021 14:59

change filters oneOf validation

81ef75e

Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com>

fix formatting

5363c2b

Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com>

antoineco approved these changes Nov 11, 2021

View reviewed changes

knative-prow-robot assigned antoineco Nov 11, 2021

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 11, 2021

pierDipi approved these changes Nov 11, 2021

View reviewed changes

knative-prow-robot assigned pierDipi Nov 11, 2021

knative-prow-robot merged commit 1e9f9f1 into knative:main Nov 11, 2021

devguyio mentioned this pull request Feb 15, 2022

Rename new trigger filters sql field to cesql #6148

Merged

5 tasks

rh-hemartin mentioned this pull request Mar 19, 2024

Add SubscriptionsAPI filters to APIServerSource #7799

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement CE Subscriptions filters #5715

Implement CE Subscriptions filters #5715

devguyio commented Sep 7, 2021 •

edited

Loading

devguyio commented Sep 8, 2021 •

edited

Loading

codecov bot commented Sep 8, 2021 •

edited

Loading

antoineco commented Sep 8, 2021

odacremolbap left a comment

devguyio commented Sep 8, 2021

antoineco left a comment

pierDipi left a comment

devguyio commented Sep 9, 2021

devguyio commented Nov 10, 2021

devguyio commented Nov 11, 2021 •

edited

Loading

knative-metrics-robot commented Nov 11, 2021

pierDipi left a comment

knative-prow-robot commented Nov 11, 2021

Implement CE Subscriptions filters #5715

Implement CE Subscriptions filters #5715

Conversation

devguyio commented Sep 7, 2021 • edited Loading

Proposed Changes

Pre-review Checklist

devguyio commented Sep 8, 2021 • edited Loading

codecov bot commented Sep 8, 2021 • edited Loading

Codecov Report

antoineco commented Sep 8, 2021

odacremolbap left a comment

Choose a reason for hiding this comment

devguyio commented Sep 8, 2021

antoineco left a comment

Choose a reason for hiding this comment

pierDipi left a comment

Choose a reason for hiding this comment

devguyio commented Sep 9, 2021

devguyio commented Nov 10, 2021

devguyio commented Nov 11, 2021 • edited Loading

knative-metrics-robot commented Nov 11, 2021

pierDipi left a comment

Choose a reason for hiding this comment

knative-prow-robot commented Nov 11, 2021

devguyio commented Sep 7, 2021 •

edited

Loading

devguyio commented Sep 8, 2021 •

edited

Loading

codecov bot commented Sep 8, 2021 •

edited

Loading

devguyio commented Nov 11, 2021 •

edited

Loading